Skip to content

Add cursor agent support, reject unknown agents, fix ACP alias collision#409

Closed
bgoosmanviz wants to merge 6 commits intoroborev-dev:mainfrom
bgoosmanviz:fix/support-for-cursor-agent
Closed

Add cursor agent support, reject unknown agents, fix ACP alias collision#409
bgoosmanviz wants to merge 6 commits intoroborev-dev:mainfrom
bgoosmanviz:fix/support-for-cursor-agent

Conversation

@bgoosmanviz
Copy link

@bgoosmanviz bgoosmanviz commented Mar 2, 2026 •
edited by wesm
Loading

Summary

  • Add cursor agent with "agent" CLI binary and "agent" → "cursor" alias
  • Reject unknown agent names (typos like --agent claud) with a clear error listing known agents, instead of silently falling back to a different agent
  • Fix ACP alias collision where acp.name = "agent" would incorrectly intercept cursor requests via alias resolution — isConfiguredACPAgentName now uses exact matching on the raw (pre-alias) name
  • Return 400 for unknown agent names and 503 for no-agents-available, using typed UnknownAgentError so HTTP handlers can distinguish the two cases
  • Add deterministic agent fallback/availability tests for both enqueue and fix endpoints, with proper registry and PATH isolation

Test plan

  • go test ./internal/agent/... -count=1 — all pass
  • go test ./internal/daemon/... -count=1 — all pass
  • go vet ./... — clean
  • TestGetAvailableRejectsUnknownAgent — typo returns error
  • TestGetAvailableFallsBackForKnownUnavailable — known agent falls back (deterministic)
  • TestACPAliasCollisionFixed — acp.name="agent" + request "cursor" → real cursor agent
  • TestACPNameDoesNotMatchCanonicalRequest — acp.name="claude" does not match "claude-code"
  • TestHandleEnqueueAgentAvailability/unknown_agent_returns_400
  • TestHandleFixJobAgentAvailability/unknown_fix_agent_returns_400
  • TestHandleFixJobAgentAvailability/no_agents_available_returns_503

🤖 Generated with Claude Code

@roborev-ci
Copy link

roborev-ci bot commented Mar 2, 2026

roborev: Combined Review (ffc4cc5)

Verdict: The PR adds support for mapping the "agent" CLI command to the "cursor" agent
, but requires a high-priority fix for a compilation error in the tests.

High Severity

  • internal/agent/agent_test.go (Lines 45-46)
    • Problem: TestCanonicalNameAliases attempts to call CanonicalName(), but the actual
      function handling alias resolution in internal/agent/agent.go is unexported and named resolveAlias(). This will result in a compilation error.
    • Fix: Change CanonicalName(tt.input) to resolveAlias(tt.input) inside the test loop.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

mariusvniekerk
mariusvniekerk previously requested changes Mar 2, 2026
View reviewed changes
Copy link
Collaborator

@mariusvniekerk mariusvniekerk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how this issue arose we need more validation on the config /params for agents. Returning an error for an unknown explicitly asked for agent feels appropriate

bgoosman and others added 4 commits March 5, 2026 07:26
@bgoosman @wesm
fix: add cursor agent support
d0d7633
@wesm @claude
fix: reject unknown agent names and fix ACP alias collision
29d47b0 
GetAvailable() now returns an error for unrecognized agent names
(typos like "claud") instead of silently falling back. Known-but-
unavailable agents (binary not installed) still fall back as before.

Fix isConfiguredACPAgentName() to use exact comparison instead of
alias-resolved comparison, preventing the "agent" → "cursor" alias
from incorrectly matching ACP config when acp.name = "agent".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wesm @claude
fix: improve test determinism and add ACP canonical-match regression …
51a16d6 
…test

Make TestGetAvailableFallsBackForKnownUnavailable deterministic by
isolating the registry and PATH instead of relying on ambient state.

Add TestACPNameDoesNotMatchCanonicalRequest to lock the contract that
acp.name="claude" matches request "claude" but not "claude-code".

Add clarifying comment to isConfiguredACPAgentName explaining exact
match semantics.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wesm @claude
fix: add Windows .exe extension to fake binary in fallback test
59dc306 
Append .exe on Windows so exec.LookPath resolves the stub binary
via PATHEXT, matching the pattern used in other agent tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wesm wesm force-pushed the fix/support-for-cursor-agent branch from ffc4cc5 to 59dc306 Compare March 5, 2026 15:53
@wesm @claude
fix: return 400 for unknown agent names, 503 for no agents available
9454aea 
Add UnknownAgentError typed error so callers can distinguish typos
(bad request) from genuinely unavailable agents (service unavailable).

Server enqueue and fix endpoints now return 400 with "invalid agent"
for unknown names instead of 503 with "no review agent available".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@roborev-ci
Copy link

roborev-ci bot commented Mar 5, 2026

roborev: Combined Review (59dc306)

Verdict: The changes successfully add Cursor alias support and fix ACP alias collisions, but introduce a regression where unknown-agent errors are misclassified as 503 HTTP status codes.

Medium

Unknown-agent errors are misclassified as 503 Service Unavailable at API level

  • Files: internal/agent/agent.go:178, internal/daemon
    /server.go:610    , [internal/daemon/server.go:1974](/home/roborev/repos/roborev/internal/daemon/server
    .go:1974)
  • GetAvailable now returns an "unknown agent" error for typos/misconfigurations, but enqueue/fix handlers still map all agent-resolution errors to 503. That turns a validation/configuration error into a transient availability error, which can cause incorrect
    retry behavior and misleading UX.
  • Suggested fix: return a typed/sentinel error from agent resolution (e.g. ErrUnknownAgent) and map it to 400 for explicit request values (or config error class), while keeping 503 for true “no installed agents available
    ”.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm @claude
fix: add fix-endpoint agent availability tests, use fmt.Sprintf
7de28c4 
Add TestHandleFixJobAgentAvailability covering the 400/503 split in
handleFixJob: unknown fix agent returns 400, no agents returns 503.

Replace unnecessary fmt.Errorf().Error() with fmt.Sprintf in
UnknownAgentError.Error() to avoid allocating a throwaway error.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wesm wesm dismissed mariusvniekerk’s stale review March 5, 2026 16:31

addressed findings

@wesm wesm changed the title fix: add cursor agent support Add cursor agent support, reject unknown agents, fix ACP alias collision Mar 5, 2026
@wesm wesm closed this Mar 5, 2026
@wesm
Copy link
Collaborator

wesm commented Mar 5, 2026

GitHub is being cranky so opened #430 so I can get CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mariusvniekerk mariusvniekerk mariusvniekerk left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bgoosmanviz @wesm @mariusvniekerk @bgoosman